Add baseline glutton#243
Conversation
8097647 to
d2290e4
Compare
What does this mean? Can you please add a longer PR description so it's clearer to potential reviewers what's happening here? |
|
Sorry, linked the issue to the PR, but not the PR to the issue. Updated description. |
Thanks :) |
d2290e4 to
d0de2a9
Compare
Bowei Du (bowei)
left a comment
There was a problem hiding this comment.
Generally lgtm. You might want to document in the RPC defs what each glutton action intends to do.
| } | ||
| } | ||
|
|
||
| func (s *gluttonService) WriteRAM(ctx context.Context, req *glutton.WriteRAMRequest) (*glutton.WriteRAMResponse, error) { |
There was a problem hiding this comment.
I don't think this makes a huge difference, but this seems to do more than what I think of for writeRAM as it allocates 2x the RAM (if the argument Size is like 32 Gb or something).
Overwrite should just touch bytes but do not extra allocation?
There was a problem hiding this comment.
This is a good point. Writing out a larger array will still require swapping the buffer, but we can at least deal with writing to subsets of the buffer.
My main hope with this one is to give us the ability to more easily see the effect of different snapshotting systems.
There was a problem hiding this comment.
would be helpful to summarize behavior in a 1-2 sentences for the WriteRAM docstring.
dd6ff79 to
1947706
Compare
|
Thanks for the review! addressed comments and rebased. |
|
Missed the documentation in the protobuf comment, will integrate into automation PR |
Bowei Du (bowei)
left a comment
There was a problem hiding this comment.
Do we want to add typing to the Python code? I know Python types are optional, but it does quickly become a maintenance issue.
| def on_start(self): | ||
| update_user_count(1, self.__class__.__name__) | ||
|
|
||
| target = self.api_host.replace("http://", "").replace("https://", "") |
There was a problem hiding this comment.
this probably deserves a comment on what this is for
There was a problem hiding this comment.
added
| self.api_channel.close() | ||
|
|
||
| def _teardown_actor(self): | ||
| try: |
There was a problem hiding this comment.
do we need to track any of these teardown failures?
There was a problem hiding this comment.
in later PRs this gets traced, also this code moves to a boomer worker (golang implementation of Locust protocol), probably not worth fixing.
| glutton_pb2.PingRequest(message=msg), | ||
| metadata=metadata, | ||
| ) | ||
| duration = (time.time() - start_time) * 1000 |
There was a problem hiding this comment.
changed
| spec: | ||
| runsc: | ||
| amd64: | ||
| url: "gs://gvisor/releases/nightly/2026-05-19/x86_64/runsc" |
There was a problem hiding this comment.
might want a comment here as to where this reference comes from?
There was a problem hiding this comment.
This goes away upon rebase (that parameter has been removed)
| defer serverboot.ShutdownProvider("MeterProvider", mp.Shutdown) | ||
|
|
||
| if err := os.MkdirAll(*dataDir, 0o700); err != nil { | ||
| serverboot.Fatal(ctx, "Failed to create data directory", err) |
There was a problem hiding this comment.
add the dataDir to the error
There was a problem hiding this comment.
added
|
|
||
| lis, err := net.Listen("tcp", *listenAddr) | ||
| if err != nil { | ||
| serverboot.Fatal(ctx, "Failed to start listener", err) |
There was a problem hiding this comment.
Is the listenAddr in the error (for debugging)
There was a problem hiding this comment.
added
| fds []*os.File | ||
| peers map[string]*peerGossip | ||
|
|
||
| ramWriteBytes metric.Int64Counter |
There was a problem hiding this comment.
Do you think it would be easier to maintain if we should split off each of these kind of operations into different structs with a abstract interface? then we wouldn't have to throw all of the operation code into this one struct.
something like
type action interface {
register()
do()
stop()
}
There was a problem hiding this comment.
personal preference to also split this into different files to avoid this become a mega file.
the other thing you will want to do is to keep the package main to a minimum and put the real logic into a testable package. You can't add unit tests to the main package.
Move the code to cmd/testing/glutton/internal...
There was a problem hiding this comment.
I think some refactoring makes sense, but I don't want to do it in this specific PR because other PRs are already built on top of it and splitting files starts to get hairy WRT rebasing. Happy to tackle this in a follow-on PR.
| } | ||
| } | ||
|
|
||
| func (s *gluttonService) WriteRAM(ctx context.Context, req *glutton.WriteRAMRequest) (*glutton.WriteRAMResponse, error) { |
There was a problem hiding this comment.
would be helpful to summarize behavior in a 1-2 sentences for the WriteRAM docstring.
1947706 to
f2f8587
Compare
|
Added doc strings. Addressed all comments except ones that will be mooted by follow-on PRs and deferring glutton refactor to avoid rebase difficulty. |
f2f8587 to
b0aa689
Compare
|
/lgtm |
Adds the glutton image to support easier load testing contemplated in #98
This is a rough implementation to help bootstrap more repeatable testing and is expected to change over time as we learn more about performance/testing requirements.